Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start filling out SQLite support #1410

Merged
merged 25 commits into from
Feb 12, 2022

Conversation

PadraigK
Copy link
Contributor

@PadraigK PadraigK commented Feb 2, 2022

This adds a few tests for SQLite and implements the necessary features to have them pass.

@@ -76,6 +76,7 @@ alter_table_stmt:
| COLUMN_? old_column_name = column_name TO_ new_column_name = column_name
)
| ADD_ COLUMN_? column_def
| DROP_ COLUMN_? column_name
Copy link
Contributor Author

@PadraigK PadraigK Feb 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this support to the parser required a re-gen, which explains a lot of the size of this PR.

@PadraigK PadraigK mentioned this pull request Feb 2, 2022
@PadraigK
Copy link
Contributor Author

PadraigK commented Feb 2, 2022

I would welcome any feedback on Go style etc. I've tried to follow the patterns I see in the existing code, but I'm new to Go and am probably missing some conventions.

Copy link
Collaborator

@kyleconroy kyleconroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit torn about improving SQLite support using the ANTLR parser. I think the best long-term approach is to use golemon and hand-port the SQLite parser.

That said, no one is working on that and this parser currently works, so I'm happy to have you continue to work on it.

Comment on lines 21 to 25
const countStarUpper = `-- name: CountStarUpper :one
r;

SELECT COUNT(*) FROM bar
`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what's happening here, but this query is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's something to do with the -- comment parsing. I'll look into it properly tomorrow!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue is fixed now and the code generates properly.

@PadraigK
Copy link
Contributor Author

PadraigK commented Feb 3, 2022

Thank you for all your work on this project, I'm really enjoying tinkering with it.

A bit torn about improving SQLite support using the ANTLR parser. I think the best long-term approach is to use golemon and hand-port the SQLite parser.

That said, no one is working on that and this parser currently works, so I'm happy to have you continue to work on it.

Interesting! I'm not against changing tack on this. I don't have a particular attachment to ANTLR, I continued with it mainly because the pieces were already in place.

Just to weigh up the pros and cons, ANTLR is a very mature project and the generated parser seems to work fine for everything I've needed so far, so I'm inclined to continue with it.

I understand that lemon is what SQLite itself uses so maybe there'll be some nuances that match up better by using golemon. Plus it's always a little nicer in terms of maintenance to have the full stack in the same language. Are there other other reasons in mind to favour golemon?

Anyway, worst case, this work I'm doing so far will build up a good library of SQLite test cases and a list of supported sql functions which will be useful with any parser.

@kyleconroy
Copy link
Collaborator

I understand that lemon is what SQLite itself uses so maybe there'll be some nuances that match up better by using golemon. Plus it's always a little nicer in terms of maintenance to have the full stack in the same language. Are there other other reasons in mind to favour golemon

Nope, the sole reason is that it would be closer to the real SQLite parser. Our PostgreSQL support is better than our MySQL support because our PostgreSQL parser is the PostgreSQL parser, where the MySQL parser is what PingCAP currently supports.

That said, if you think the current parser works, I'm all for it.

@kyleconroy kyleconroy merged commit 323187f into sqlc-dev:main Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants